✨ Support IPv6/dual-stack routable pods with NSX-VPC#1763
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: silvery1622 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/hold |
k8s-ci-robot
added
size/XXL
silvery1622
force-pushed
the
ipv6-dualstack-routablepods
branch
from
67393ba to
45e7c24
Compare
silvery1622
force-pushed
the
ipv6-dualstack-routablepods
branch
4 times, most recently
from
1c90c66 to
54c942f
Compare
| // We deliberately do NOT parse the CR name to decide the family: node names | ||
| // that themselves end in "-ipv6" would otherwise be misclassified. | ||
| func isIPv4Allocation(alloc *vpcapisv1.IPAddressAllocation) bool { | ||
| return alloc.Spec.IPAddressType != vpcapisv1.IPAllocationIPAddressTypeIPv6 |
There was a problem hiding this comment.
According to method name, alloc.Spec.IPAddressType == vpcapisv1.IPAllocationIPAddressTypeIPv4 looks better
There was a problem hiding this comment.
We cannot use == vpcapisv1.IPAllocationIPAddressTypeIPv4 because of backward compatibility. Older CRs created before dual stack support have IPAddressType unset (empty string). By checking != IPv6, we correctly classify both explicitly set IPv4 CRs and older empty-type CRs as IPv4, which matches the upstream API default.
| } else { | ||
| // Dual-stack: list all allocations for this node. | ||
| // Dual-stack clusters are new and guaranteed to have the nodeName label on their CRs. | ||
| selector := labels.SelectorFromSet(labels.Set{helper.LabelKeyNodeName: nodeName}) |
There was a problem hiding this comment.
Should we also add label for cluster name or vsphere cluster name? There could be same nodeName in different clusters, for example, below 2 clusters all render node name coffee-worker-beijing
cluster coffee nodepool worker-beijing
cluster coffee-worker nodepool beijing
There was a problem hiding this comment.
Thanks for the comment. I have updated the code and add clusterName into IPAddressAllocation's label.
I also passed clusterName into IPAddressAllocation Controller and matches both nodeName and clusterName when doing label selector.
| Namespace: m.svNamespace, | ||
| Labels: map[string]string{ | ||
| helper.LabelKeyNodeName: nodeName, | ||
| helper.LabelKeyIPFamily: familyLabel, |
There was a problem hiding this comment.
The spec.IPAddressType contains ipfamily so I don't think this label is used anywhere. We need to add a label for VSphereCluster
There was a problem hiding this comment.
Agreed. I have replaced the IPFamily label with clusterName label.
| ctx := context.TODO() | ||
|
|
||
| for _, ipv4 := range families { | ||
| crName := crNameForFamily(node.Name, ipv4) |
There was a problem hiding this comment.
We should use label to get IPAddressAnnotation rather than name.
There was a problem hiding this comment.
We need continue using Get(crName) to ensure backward compatibility for existing IPv4 single stack clusters during upgrades, as their CRs lack the new labels. Since dual stack is greenfield only, the deterministic CR name crNameForFamily(nodeName, ipv4) is guaranteed to be unique and safe to use without label filtering.
| // deleteIPAddressAllocation deletes the IPAddressAllocation CR for the given node and | ||
| // IP family. NotFound is treated as success (idempotent delete). | ||
| func (m *NSXVPCIPManager) deleteIPAddressAllocation(ctx context.Context, nodeName string, ipv4 bool) error { | ||
| crName := crNameForFamily(nodeName, ipv4) |
There was a problem hiding this comment.
We need continue using Get(crName) to ensure backward compatibility for existing IPv4 single stack clusters during upgrades, as their CRs lack the new labels. Since dual stack is greenfield only, the deterministic CR name crNameForFamily(nodeName, ipv4) is guaranteed to be unique and safe to use without label filtering.
| // destination is actually IPv6, so an IPv4 route on a node whose | ||
| // name happens to end in "-ipv6" is not silently truncated. | ||
| nodeName := staticroute.Labels[helper.LabelKeyNodeName] | ||
| if nodeName == "" { |
There was a problem hiding this comment.
Raise an error till label is added by node controller.
There was a problem hiding this comment.
We cannot raise an error here because it would break route syncing for existing IPv4 single stack clusters during upgrades, as their legacy CRs lack the label. However, since dual stack is greenfield only, any CR without the label is guaranteed to be a legacy IPv4 CR. I have simplified the code to safely fall back to the CR name directly.
| } | ||
| if podCIDR == "" { | ||
| return fmt.Errorf("IPAddressAllocation %v does not get CIDR allocated", ipAddressAllocation.Name) | ||
| return helper.StripFamilySuffix(alloc.Name) |
There was a problem hiding this comment.
Do not fallback, Raise an error till label is added by node controller.
There was a problem hiding this comment.
Similar to StaticRoute, raising an error would break backward compatibility for existing IPv4 single stack clusters. Since dual stack is greenfield only, any CR lacking the label is guaranteed to be a legacy IPv4 CR. I have simplified the fallback logic to just return the CR name directly.
…NSX-VPC - hack/nsx-operator-apis-fork: local copy of nsx-operator API types with updated IPAddressAllocation (multi-family CIDR fields) - routablepod/core.go: thread ipv4Enabled/ipv6Enabled/primaryIPFamilyIsIPv4 through to IPAddressAllocation and node controllers - routablepod/ipaddressallocation: allocate separate IPv4/IPv6 CRs in dual-stack - nsxipmanager/nsx_vpc.go: reconcile IPv4/IPv6 IPAddressAllocation objects - routemanager: propagate per-family CIDRs to StaticRoute CRs - cloud.go: derive IP-family flags from --cluster-ip-family and pass to StartControllers
silvery1622
force-pushed
the
ipv6-dualstack-routablepods
branch
from
54c942f to
090247e
Compare
3 participants
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: